backup: Reduce the number of GetBlobs. - #503
Conversation
* When getting an object out of the cache it's not needed to recheck that it exists in the repo. This used to be true because the vfs cache was local and so it could be lying (repo changed due to another user). Now that we base the vfs cache on a previous snapshot we know _by definition_ that the object exists and so BlobExists will always be true. * BlobExists is a "slow" function and is the hottest path of them all, reducing its usage is always a win. * On a non changing backup this divides by 4 the number of calls we emit to the state cache. * This is a scary diff, I've been sitting on it for a while, so this needs to get some extra considerations from reviewers.
There was a problem hiding this comment.
Pull request overview
This PR reduces repository BlobExists lookups on the backup hot path by trusting the VFS cache (seeded from a previous snapshot) and reusing cached object/entry MACs without re-validating their presence in the repository.
Changes:
- Removed
BlobExists(RT_OBJECT, ...)checks when reusing cached content objects. - Removed
BlobExists(RT_VFS_ENTRY, ...)checks when reusing cached VFS directory/file entry MACs. - Keeps the rest of the backup pipeline (scanlog -> indexes -> persist) unchanged, relying on cached blobs already being present.
Comments suppressed due to low confidence (2)
snapshot/backup.go:861
- writeDirectoryEntry now reuses cachedPath.MAC without verifying that the referenced RT_VFS_ENTRY blob exists in the destination repository. This removes the only safety net when the VFS cache is accidentally sourced from a different repo (or otherwise not guaranteed to be present), which can persist a VFS btree pointing at missing entry blobs.
if cachedPath != nil {
dirEntryMAC = cachedPath.MAC
serialized, err := dirEntry.ToBytes()
if err != nil {
return err
snapshot/backup.go:902
- writeFileEntry now trusts cachedPath.MAC (RT_VFS_ENTRY) on cache hits without confirming it exists in the destination repository. If the VFS cache comes from another repository (possible because WithVFSCache does not validate) this will record file entries whose VFS_ENTRY blobs are missing, yielding a corrupted snapshot.
if cachedPath != nil {
fileEntryMAC = cachedPath.MAC
if fileEntry.Object == (objects.MAC{}) && cachedPath.ObjectMAC != (objects.MAC{}) {
fileEntry.Object = cachedPath.ObjectMAC
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
one question, cause I can't wrap my head around that: if we move to a model where we allow keeping a local dirpack cache, doesn't this reintroduce the need to check the remote store ? my question is essentially, is this removal always valid, if not, does it mean that we need to track at the ctx level if we're using a local cache vs a remote cache and bypass the check as an optimization of the remote cache ? |
poolpOrg
left a comment
poolpOrg
left a comment
There was a problem hiding this comment.
makes sense, LGTM modulo my comment
When getting an object out of the cache it's not needed to recheck that it exists in the repo. This used to be true because the vfs cache was local and so it could be lying (repo changed due to another user). Now that we base the vfs cache on a previous snapshot we know by definition that the object exists and so BlobExists will always be true.
BlobExists is a "slow" function and is the hottest path of them all, reducing its usage is always a win.
On a non changing backup this divides by 4 the number of calls we emit to the state cache.
This is a scary diff, I've been sitting on it for a while, so this needs to get some extra considerations from reviewers.